Repository navigation
Conversation
|
The problem with relying on Also you have given the repair logic a lie: The prices are still in GBp not GBP, If a switch is detected without volume jump, then it's a unit-switch and the only truth is:
This should be sufficient. |
|
Both points taken, and fixed by deleting the anchor rather than defending it. On the false positives: agreed, On the fixture: the 50s-with-a-4800-block table was meant as the state after The direction now comes from your rule. A switch that passed the no-volume-jump gate is a unit switch, so the bigger numbers are the minor currency and the smaller the major. When a lone range would divide everything from some date back to the oldest row in a table standardisation actually divided, the bigger older side is by that rule already at major scale, and the smaller newest segment is what standardisation wrongly divided: the repair multiplies that segment up directly. The whole-table revert and its flag inversion are deleted, net -19 lines in history.py. Your rule needed one recorded bit to apply after the pipeline has run: Six offline cases in total. Three fail on current dev: the interior block (the whole table rescaled, flags inverted), the relabel-only switch (the revert un-repairs a correct repair), and the Pushed as 1ac1130, PR description updated to match. |
| del ranges[i] | ||
| if unit_switch and len(ranges) == 1 and ranges[0][1] == n \ | ||
| and self._history_metadata.get('currencyRepaired', False) \ | ||
| and self._history_metadata.get('currencyScaled', True): |
There was a problem hiding this comment.
You're right, that state can't exist. _standardise_currency() is the only writer and always sets the two keys together, the post-repair revert deletes them together, and every fetch rebuilds the metadata from Yahoo's raw response, so the pair can never disagree. The compound check was guarding key absence, not that combination: currencyRepaired (default False) kept the gate closed for currencies standardisation never touches, against the True default on currencyScaled. That is the wrong shape for a question one flag answers, so the gate is now the single check self._history_metadata.get('currencyScaled', False), defaulting closed. Equivalent in every reachable state, and the offline cases pin all three: divided, relabel-only, and never-standardised. Pushed as 0fe6ad3.
|
Ignore that review, I was being dumb. This seems fine. But before merging I'm going to stress-test it against my cached yf data. |
|
Rebase to latest dev, just in case this commit affects e285bd1 |
0fe6ad3 to
ebf713e
Compare
|
Rebased onto f854309. The three commits are unchanged (range-diff clean); one new commit adapts the offline metadata test to |
|
I saw you reply to my review with:
But my comment was nonsense. Are you AI? |
|
I'm a person, 100%. I run a Claude Code loop that analyzes repos and drafts the fixes. I review what it produces and I only open a PR when I think the finding is genuinely useful. That reply agreeing with your review is one I didn't read closely enough before it went out, and that's on me. |
ebf713e to
ec5fc19
Compare
|
@ValueRaider no rush, just checking whether the stress test turned anything up, and whether a rebase onto dev would help. |
|
I'm not seeing this have any effect in my own pipeline - it runs on yfinance-cache, so I would expect to see 100x/drops jumps as I add to cache with your change. I want to see a real-world change from this. |
…ocks The revert added in d3704c8 fires whenever any row was repaired downward after _standardise_currency() ran. A genuine partial 100x block inside an already-converted table then gets the whole table rescaled 100x and the Repaired? flags inverted. Distinguish the two cases with the same anchor _standardise_currency() already uses: after this pass's repairs, a re-applied conversion leaves the table ~change-x below regularMarketPrice, a repaired partial block leaves it at the market level. Revert only in the first case. Adds fixture-based tests for both scenarios, and a guard for _repair_capital_gains raising ZeroDivisionError when a fund distributes capital gains with no dividend that day (or only on the first row).
…arMarketPrice Review feedback on the previous commit was right twice over: anchoring on regularMarketPrice assumes the fetch window ends near today, and the test fed the anchor a value in the wrong unit. Both problems go away by removing the anchor entirely. In a genuine unit switch the bigger numbers are the minor currency and the smaller the major. So when a lone repair range divides everything from some date back to the oldest row in a table _standardise_currency() actually divided, the switch is genuine and standardisation wrongly divided the newest segment: repair that segment up directly, instead of dragging the correct majority down and reverting the whole table afterwards. The whole-table revert and its flag inversion are deleted; interior 100x blocks repair locally as before. _standardise_currency() now records whether it divided (currencyScaled), because it sets currencyRepaired even when it only relabels, and the two states need opposite repair directions. Tests cover all four states, no regularMarketPrice anywhere: a genuine interior 100x block, an interior block standardisation wrongly divided, a quotation switch standardisation wrongly divided (bigger side already major), and the same switch when standardisation only relabelled (older pence side divided down).
currencyScaled is only ever written alongside currencyRepaired, so gating on both keys with opposite defaults was redundant and read as if the pair could disagree. One check, defaulting closed, is equivalent in every reachable state.
f854309 dropped div_scaled from the return tuple. The test unpacked four values with the third discarded, so it raised ValueError on the rebase. Unpack three.
ec5fc19 to
f6ae542
Compare
|
CNE.L (Capricorn Energy, quoted in GBp) shows it on Raw closes (
|
Fixes the main finding in #2924, plus the capital-gains
ZeroDivisionErrorconfirmed still present on dev.The defect. The revert in
_fix_prices_sudden_change(added in d3704c8 to stop the unit-switch repair applying twice) fires whenever any row was repaired downward and_standardise_currency()had run. When the table instead contains a genuine interior 100x block, the block repair is correct, and the revert then rescales the whole table 100x and invertsRepaired?: 90 of 90 rows wrong, the 20 broken rows flagged clean, the 70 clean rows flagged repaired.The fix. No external anchor, and no revert. A switch that passed the no-volume-jump gate is a unit switch, so the bigger numbers are the minor currency and the smaller the major. When a lone range would divide everything from some date back to the oldest row, in a table
_standardise_currency()actually divided, the bigger older side is by that rule already at major scale and the smaller newest segment is what standardisation wrongly divided: repair that segment up directly. The whole-table revert and its flag inversion are deleted; interior blocks repair locally without consulting metadata._standardise_currency()now records whether it divided (currencyScaled), because it setscurrencyRepairedeven when it only relabels GBp to GBP, and the two states need opposite repair directions. Net -19 lines inhistory.py.Tests. All offline, none carries
regularMarketPrice. The unit-switch cases drive_fix_unit_switchon CSV fixtures readable as a spreadsheet, in the exact state_standardise_currency()leaves behind:partial-100x-blockwrongly-divided-blockreapplied-conversionswitch-not-dividedprices_in_subunitsFalse)mixed-units-no-standardisecurrencyRepairedpartial-100x-block- interior repair independent of metadatatest_standardise_currency_records_scaling_standardise_currencycurrencyScaledTrue after a real division, False after a relabel - fails on devZeroDivisionError- fails on devtests.test_price_repair: every pre-existing test has the same outcome on this branch as on dev - the failures on both sides are the live-fetch tests hitting rate limits in my environment, everything else passes on both.